Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib, treewide: introduce repoRevToName and use it to cleanup most fetch* functions #316668

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oxij
Copy link
Member

@oxij oxij commented Jun 2, 2024

This a first half of #49862. Basically, this is a noop cleanup that keeps all the fetch* names as they are now, while introducing the new config.nameSourcesPrettily option.

Changes

lib, treewide: introduce repoRevToName, use it in most fetch* functions

This patch adds lib.repoRevToName function that generalizes away most of the code used for derivation name generation by fetch* functions (fetchzip, fetchFromGitHub, etc, except those which are delayed until latter commits for mass-rebuild reasons).

It's first argument controls how the resulting name will look (see below).

Since lib has no equivalent of Nixpkgs' config, this patch adds config.nameSourcesPrettily option to Nixpkgs and then re-exposes lib.repoRevToName config.nameSourcesPrettily expression as pkgs.repoRevToNameMaybe which is then used in fetch* derivations.

The result is that different values of config.nameSourcesPrettily now control how the src derivations produced by fetch* functions are to be named, e.g.:

  • nameSourcesPrettily = false (the default):

      $ nix-instantiate -A fuse.src
      /nix/store/<hash>-source.drv
    
  • nameSourcesPrettily = true:

      $ nix-instantiate -A fuse.src
      /nix/store/<hash>-libfuse-2.9.9-source.drv
    
  • nameSourcesPrettily = "full":

      $ nix-instantiate -A fuse.src
      /nix/store/<hash>-libfuse-2.9.9-github-source.drv
    

See documentation of config.nameSourcesPrettily for more info.

fetchsvn: move the name generator outside of the thunk

This is slightly more efficient.

@oxij oxij requested a review from infinisil as a code owner June 2, 2024 13:40
@github-actions github-actions bot added 6.topic: fetch 6.topic: lib The Nixpkgs function library labels Jun 2, 2024
@oxij oxij changed the title lib, treewide: introduce repoRevToName and use to cleanup most fetch* functions lib, treewide: introduce repoRevToName and use it to cleanup most fetch* functions Jun 2, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Jun 2, 2024
lib/sources.nix Outdated Show resolved Hide resolved
@@ -254,6 +254,64 @@ let
outPath = builtins.path { inherit filter name; path = origSrc; };
};

# urlToName : (URL | Path | String) -> String
#
# Transform a URL (or path, or string) into a clean package name.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I can be wrong, but the general term is URI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a thing more general than a URL? Yes. But here it is literally "a URL, or a path, or a string (a package name, usually)".

lib/sources.nix Outdated Show resolved Hide resolved
# /nix/store paths.
#
# This uses a different implementation depending on the `pretty` argument:
# false -> name everything as "source"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I do not like dual-type attributes. This is a serious footgun in a duck-typed language.

And let's remember Nix is not stellar on the error report department:
https://discourse.nixos.org/t/rant-finding-errors-in-module-configs-after-incompatible-upgrades-is-dreadful/46375

Either is everything a string, say format ? "short", "medium", "long" or we stick to only two options (and then we can use a Boolean).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it is not checked by Nix but by the config.nix and NixOS typing infra, and the errors it generates are pretty nice, e.g. with nameSourcesPrettily = "error":

$ nix-instantiate ./default.nix -A xlife
error: A definition for option `nameSourcesPrettily' is not of type `boolean or value "full" (singular enum)'. Definition values:
- In `nixpkgs.config': "error"
(use '--show-trace' to show detailed location information)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, less bad.

Nonetheless, semantically it is a clear case for an enumeration with three possible states, instead of a weird Boolean-plus-an-extra.

@@ -50,6 +50,39 @@ let
default = false;
};

nameSourcesPrettily = mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nameSourcesPrettily = mkOption {
usePrettierSourceNames = mkOption {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, but why? nameSourcesPrettily is shorter and means the same thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's shorter because it is more obscure.
verb-adjective-name is clearer than name-name-adverb.

On the other hand, it depends on the previous tristate vs boolean-plus-state discussion.

oxij added 2 commits June 5, 2024 11:46
…ctions

This patch adds `lib.repoRevToName` function that generalizes away most of the
code used for derivation name generation by `fetch*` functions (`fetchzip`,
`fetchFromGitHub`, etc, except those which are delayed until latter commits
for mass-rebuild reasons).

It's first argument controls how the resulting name will look (see below).

Since `lib` has no equivalent of Nixpkgs' `config`, this patch adds
`config.nameSourcesPrettily` option to Nixpkgs and then re-exposes
`lib.repoRevToName config.nameSourcesPrettily` expression as
`pkgs.repoRevToNameMaybe` which is then used in `fetch*` derivations.

The result is that different values of `config.nameSourcesPrettily` now
control how the `src` derivations produced by `fetch*` functions are to be
named, e.g.:

- `nameSourcesPrettily = false` (the default):

  ```
  $ nix-instantiate -A fuse.src
  /nix/store/<hash>-source.drv
  ```

- `nameSourcesPrettily = true`:

  ```
  $ nix-instantiate -A fuse.src
  /nix/store/<hash>-libfuse-2.9.9-source.drv
  ```

- `nameSourcesPrettily = "full"`:

  ```
  $ nix-instantiate -A fuse.src
  /nix/store/<hash>-libfuse-2.9.9-github-source.drv
  ```

See documentation of `config.nameSourcesPrettily` for more info.
@oxij oxij force-pushed the tree/repo-to-name-part-1 branch from 38ffa76 to a09a1e7 Compare June 5, 2024 11:48
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: fetch 6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants